-
Notifications
You must be signed in to change notification settings - Fork 374
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CockroachDB Support #359
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also like to see tests for the cockroachdb dialect.
@@ -72,6 +72,9 @@ type Dialect interface { | |||
IfSchemaNotExists(command, schema string) string | |||
IfTableExists(command, schema, table string) string | |||
IfTableNotExists(command, schema, table string) string | |||
|
|||
// The command to create a new database/schema | |||
CreateSchemaCommand() string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, this would force a major version bump, because custom dialects would no longer implement the updated Dialect
type. Can we do this as an optional type that dialects are not required to implement (and then use the old logic as a fallback)? Maybe something like type NonstandardSchemaDialect interface { CreateSchemaCommand() string }
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent idea. I'd like to change it slightly, though, since even a NonstandardSchemaDialect
could vary amongst custom implementations. Maybe a type SchemaCreator() interface { CreateSchemaCommand() string }
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think SchemaCreator
is a little misleading, since all other dialects will still be able to create schemas. What we're providing is a way for dialects to inform gorp
that they are unable to execute create schema
operations per the SQL standard.
For those reasons, I still think something along the lines of "nonstandard" should be part of the name. It is accurate - these are dialects that cannot handle the SQL standard for create schema
, so they should be labeled as such. NonstandardSchemaCreator
sounds fine to me, too.
Also, we might want to pass the schema name in to CreateSchemaCommand
so that dialects have more power over the resulting SQL command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the sound of that. I'll get that implemented and try to sort out testing after the holidays 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zikes Hey, just checking in; have you had any more time to work on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't, sorry. It's something I'd still be interested in implementing sometime, but it's been less of a priority for me lately.
Hi folks, just wondering if anyone has time to revisit this? |
Because CockroachDB does not support the
create schema
syntax, an addition to theDialect
interface was necessary.